-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Analyze Text #336
Conversation
WalkthroughThe changes introduce new functionality in the Deepgram SDK by adding methods for analyzing text input. The Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (8)
Deepgram/Models/Analyze/v1/TextSource.cs (1)
7-8
: LGTM: Well-structured class declaration with a minor suggestion.The
TextSource
class is correctly declared as public, making it accessible for SDK users. The use of a primary constructor is a concise and modern approach.Consider adding a null check in the constructor to ensure that the
text
parameter is not null. This can be done using the null-coalescing operator:public class TextSource(string text) { public string? Text { get; set; } = text ?? throw new ArgumentNullException(nameof(text)); // ... rest of the class }This change would prevent the creation of a
TextSource
object with a null text value, which might lead to unexpected behavior later.Deepgram/Clients/Interfaces/v1/IAnalyzeClient.cs (1)
21-27
: Enhance XML documentation for better clarity.The XML documentation for the
AnalyzeText
method is good, but it could be improved to provide more details about the parameters and return value. Consider adding descriptions for all parameters and the return value.Here's a suggested improvement for the XML documentation:
/// <summary> /// Analyzes provided text synchronously. /// </summary> /// <param name="source">Text that is to be analyzed.</param> /// <param name="analyzeSchema">Options for the analysis. Can be null.</param> /// <param name="cancellationToken">Token to cancel the operation. Optional.</param> /// <param name="addons">Additional parameters for the analysis. Optional.</param> /// <param name="headers">Custom headers to be included in the request. Optional.</param> /// <returns>A Task representing the asynchronous operation, containing a SyncResponse with the analysis results.</returns>Deepgram/Clients/Analyze/v1/Client.cs (3)
46-70
: LGTM! Consider enhancing the method documentation.The
AnalyzeText
method is well-implemented and consistent with the existing pattern in the class. It includes proper logging, callback verification, and uses thePostAsync
method as expected.Consider adding a brief description of the
cancellationToken
,addons
, andheaders
parameters in the XML documentation to provide more clarity on their usage./// <summary> /// Analyze by providing text /// </summary> /// <param name="source">Text that is to be analyzed <see cref="TextSource"></param> /// <param name="analyzeSchema">Options for the transcription <see cref="AnalyzeSchema"/></param> +/// <param name="cancellationToken">Optional cancellation token to cancel the operation</param> +/// <param name="addons">Optional dictionary of additional parameters</param> +/// <param name="headers">Optional dictionary of custom headers to include in the request</param> /// <returns><see cref="SyncResponse"/></returns>
185-214
: LGTM! Consider enhancing documentation and error handling.The
AnalyzeTextCallBack
method is well-implemented and consistent with the existing pattern in the class for asynchronous operations with callbacks. It includes proper logging, callback verification, and uses thePostAsync
method as expected.
- Enhance the method documentation:
/// <summary> /// Analyze by providing text and a CallBack /// </summary> -/// <param name="source">Text that is to be analyzed <see cref="UrlSource"/></param> +/// <param name="source">Text that is to be analyzed <see cref="TextSource"/></param> /// <param name="callBack">CallBack url</param> /// <param name="analyzeSchema">Options for the transcription<see cref="AnalyzeSchema"></param> +/// <param name="cancellationToken">Optional cancellation token to cancel the operation</param> +/// <param name="addons">Optional dictionary of additional parameters</param> +/// <param name="headers">Optional dictionary of custom headers to include in the request</param> /// <returns><see cref="AsyncResponse"/></returns>
- Consider adding null checks for
analyzeSchema
:+ analyzeSchema ??= new AnalyzeSchema(); VerifyOneCallBackSet(nameof(AnalyzeTextCallBack), callBack, analyzeSchema); if (callBack != null) analyzeSchema.CallBack = callBack;
This ensures that
analyzeSchema
is never null when used, preventing potential null reference exceptions.
Line range hint
1-250
: Overall, excellent implementation with good consistency.The new methods
AnalyzeText
andAnalyzeTextCallBack
are well-integrated into the existingClient
class. They follow the established patterns for synchronous and asynchronous operations, maintaining consistency in logging, error handling, and method structure.For consistency, consider updating the
VerifyOneCallBackSet
method to use string interpolation for error messages, similar to other parts of the code:- var exStr = "CallBack should be set in either the CallBack parameter or AnalyzeSchema.CallBack not in both."; + var exStr = $"CallBack should be set in either the CallBack parameter or AnalyzeSchema.CallBack not in both."; Log.Error("VerifyNoCallBack", $"Exceptions: {exStr}");This minor change would align the error message formatting with the rest of the class.
Deepgram.Tests/UnitTests/ClientTests/AnalyzeClientTests.cs (3)
229-251
: Consider explicitly settingCallBack
to a non-null value.The test case is well-structured and verifies the correct exception is thrown. However, to make the test more explicit and robust, consider setting the
CallBack
property to a non-null value before invoking the method.You could add the following line before the Act and Assert section:
var analyzeSchema = new AutoFaker<AnalyzeSchema>().Generate(); + analyzeSchema.CallBack = "http://example.com/callback";
This ensures that the test is explicitly checking the scenario where
CallBack
is not null.
288-316
: LGTM! Consider updating the method name for accuracy.The test case is well-implemented and correctly verifies the behavior of
AnalyzeTextCallBack
when using a callback property. The setup, mocking, and assertions are appropriate.There's a minor inconsistency in the method name. Consider updating it to reflect that an
AsyncResponse
is returned:- public async Task AnalyzeTextCallBack_Should_Call_PostAsync_Returning_SyncResponse_With_CallBack_Property() + public async Task AnalyzeTextCallBack_Should_Call_PostAsync_Returning_AsyncResponse_With_CallBack_Property()This change would make the method name more accurate and consistent with the actual behavior being tested.
343-366
: LGTM! Consider updating the expected response type for consistency.The test case is well-implemented and correctly verifies that
AnalyzeTextCallBack
throws anArgumentException
when no callback is set. The setup, mocking, and assertions are appropriate.There's a minor inconsistency in the expected response type. Consider updating it to
AsyncResponse
for consistency with other tests:- var expectedResponse = new AutoFaker<SyncResponse>().Generate(); + var expectedResponse = new AutoFaker<AsyncResponse>().Generate(); - analyzeClient.When(x => x.PostAsync<AnalyzeSchema, SyncResponse>(Arg.Any<string>(), Arg.Any<AnalyzeSchema>())).DoNotCallBase(); - analyzeClient.PostAsync<AnalyzeSchema, SyncResponse>(url, Arg.Any<AnalyzeSchema>()).Returns(expectedResponse); + analyzeClient.When(x => x.PostAsync<AnalyzeSchema, AsyncResponse>(Arg.Any<string>(), Arg.Any<AnalyzeSchema>())).DoNotCallBase(); + analyzeClient.PostAsync<AnalyzeSchema, AsyncResponse>(url, Arg.Any<AnalyzeSchema>()).Returns(expectedResponse); - await analyzeClient.DidNotReceive().PostAsync<AnalyzeSchema, SyncResponse>(url, Arg.Any<AnalyzeSchema>()); + await analyzeClient.DidNotReceive().PostAsync<AnalyzeSchema, AsyncResponse>(url, Arg.Any<AnalyzeSchema>());This change would make the test more consistent with the other
AnalyzeTextCallBack
tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- Deepgram.Tests/UnitTests/ClientTests/AnalyzeClientTests.cs (1 hunks)
- Deepgram/Clients/Analyze/v1/Client.cs (3 hunks)
- Deepgram/Clients/Interfaces/v1/IAnalyzeClient.cs (2 hunks)
- Deepgram/Models/Analyze/v1/TextSource.cs (1 hunks)
🔇 Additional comments (6)
Deepgram/Models/Analyze/v1/TextSource.cs (1)
1-5
: LGTM: Proper file header and namespace declaration.The file header includes the necessary copyright and license information. The namespace
Deepgram.Models.Analyze.v1
is correctly declared, following the expected structure for the Deepgram SDK.Deepgram/Clients/Interfaces/v1/IAnalyzeClient.cs (1)
21-27
: Approve changes with a minor suggestion for consistency.The new
AnalyzeText
andAnalyzeTextCallBack
methods are well-integrated into theIAnalyzeClient
interface, maintaining consistency with existing methods and fulfilling the PR objectives. They follow the established pattern of providing both synchronous and asynchronous (callback) versions for each analysis type.For improved consistency across the interface, consider:
- Using
callback
(lowercase 'b') instead ofcallBack
in all method signatures and documentation.- Ensuring that all methods in the interface have equally detailed XML documentation.
These minor adjustments will enhance the overall coherence and maintainability of the interface.
Also applies to: 82-89
Deepgram.Tests/UnitTests/ClientTests/AnalyzeClientTests.cs (4)
197-227
: LGTM! Well-structured test forAnalyzeText
method.The test case is well-implemented and consistent with the existing test structure. It correctly verifies the behavior of the
AnalyzeText
method, including the call toPostAsync
and the returnedSyncResponse
.
253-286
: LGTM! Well-implemented test forAnalyzeTextCallBack
with callback parameter.This test case is well-structured and correctly verifies the behavior of
AnalyzeTextCallBack
when using a callback parameter. The setup, mocking, and assertions are appropriate, and the comment explaining the nulling of theCallBack
property is helpful for understanding the test logic.
318-341
: LGTM! Well-implemented test for invalid callback configuration.This test case is well-structured and correctly verifies that
AnalyzeTextCallBack
throws anArgumentException
when both the callback property and parameter are set. The setup, mocking, and assertions are appropriate, ensuring that the method behaves correctly in this edge case.
197-366
: Overall, excellent implementation of text analysis tests.The new test methods for
AnalyzeText
andAnalyzeTextCallBack
are well-structured, comprehensive, and consistent with the existing test patterns in the codebase. They provide good coverage for both successful scenarios and error cases.A few minor suggestions were made to improve clarity and consistency:
- Explicitly setting the
CallBack
property in theAnalyzeText_Should_Throw_ArgumentException_If_CallBack_Not_Null
test.- Updating method names and expected response types for accuracy and consistency.
These small changes will further enhance the already high-quality test suite. Great job on implementing these new tests!
Hi @kaller01 This is amazing! and you even added UnitTests for the new code! Awesome! I will take a look at this today. Since there are other things in the pipeline to be released, I will see about getting this in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! @kaller01 thanks for the contribution!!!!
Proposed changes
Addresses #335, implements
AnalyzeText
,AnalyzeTextCallback
and tests following the same structure as the equivalent methods for URL.Types of changes
What types of changes does your code introduce to the community .NET SDK?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
AnalyzeClientTests
have not been linted as it currently is not using repo standards (could include that in this PR or open seperate issue?)Summary by CodeRabbit
New Features
AnalyzeText
for synchronous processing andAnalyzeTextCallBack
for asynchronous processing with callback support.TextSource
class to encapsulate text input for analysis.Tests
AnalyzeClient
class, validating the behavior of new methods under various conditions.Documentation